Repository navigation
Blob: return only the slice when consuming a slice's stream after the Blobs were collected - #38685
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughByte-backed Blob conversion now requires a unique store reference and a Blob view that covers the full store. Regression tests check slice and whole-Blob stream results after source Blob collection. ChangesBlob Slice Stream Handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change has no identified merge-blocking issue; it is ready for normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: rebased onto main (f4d755a), head 255bd1a. The tests cover the three body cases that are new on main, and windows at both edges of the store. Reproduced with CI ran two times, and no red test reads a Blob:
Each macOS lane passed in one of the two runs. The second head differs from the first by the name of one boolean and by added tests. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix narrows an optimization's precondition so a windowed Any::Blob falls through to the same to_string/to_array_buffer_view/to_json path that already handles it correctly whenever refcount > 1.
Reviewed: the new guard's types (offset/size/bytes.len() are all SizeType = u64, so the < comparison is sound); the let ... else refactor is behavior-preserving for every non-matching arm; the immutable bytes borrow ends at the len() read before data_mut() is called. Confirmed ByteBlobLoader::to_any_blob is the producer that sets offset/size = remain on the windowed path (line 158-159), matching the guard. Checked the >= rationale against shared_view's clamping — an oversized size still views the whole store, so the optimization stays enabled for slice(0). The 21 tests cover the source×consumer matrix plus nested/empty/unsliced controls; the FinalizationRegistry + gcTick poll loop is the harness-standard pattern for waiting on finalization.
Extended reasoning...
Overview
One-function change in src/runtime/webcore/Blob.rs: Any::to_internal_blob_if_possible now also requires offset == 0 && size >= bytes.len() before moving a Bytes store's whole buffer into an InternalBlob. The nested if let chain is refactored to let ... else early returns. A new 21-case describe block in test/js/web/fetch/blob.test.ts covers 2 stream sources × 8 buffered consumers, plus Response.text() after .body, a nested slice, an empty slice, and typed/untyped unsliced controls.
Security risks
None. This is a data-correctness fix in Blob stream consumption. No auth, crypto, permissions, or untrusted-input parsing is touched. The change is purely additive to a guard — it makes an optimization apply in strictly fewer cases, and the fallback is the existing windowed-Blob path (shared_view_raw) that already runs whenever another reference keeps the store alive.
Level of scrutiny
Native runtime code touching a refcount-gated buffer-adoption path, so I traced the types and the fallback carefully. SizeType is u64 (src/jsc/webcore_types.rs:32); Blob.offset/Blob.size are Cell<SizeType> (webcore_types.rs:121-122); Bytes::len() returns SizeType (webcore_types.rs:730) — the comparison is well-typed with no truncation. The fix is at the layer that owns the invariant (to_internal_blob yields the whole buffer, so the caller must prove the Blob views the whole buffer). When the guard rejects, to_action_value hits the Any::Blob arms (lines 6418-6450), which are exactly what runs today when has_one_ref() is false — a proven-correct path. No new allocation, refcount, or lifetime edge is introduced; the store still drops via the *self = ... replace when the optimization does apply, and via the stream detaching its Any when it doesn't.
Other factors
The PR description documents fail-before/pass-after (USE_SYSTEM_BUN=1, 17/21 fail across 12 runs; all 21 pass on the build) and lists a dozen adjacent test files that still pass. No CODEOWNERS cover these paths. The bug hunting system found nothing. The >= (not ==) choice is justified by shared_view's clamping and preserves the optimization for slice(0) and unsliced Blobs. The test's while (collected < tracked) await gcTick() is an unbounded loop in form, but gcTick forces a full GC and yields, and the tracked Blobs are unreachable once make returns — the file's default timeout backstops it. Tests were placed in the existing blob.test.ts per repo convention.
…ence Any::to_internal_blob_if_possible turned any Blob over a uniquely owned bytes store into an InternalBlob holding the whole buffer, ignoring the Blob's offset and size. A stream made from blob.slice() reaches it with exactly that shape once the parent and slice objects have been collected, so Bun.readableStreamToText() and the other buffered consumers returned every byte of the parent instead of the slice. Only convert when the Blob views the whole store; otherwise the Blob read paths apply the window.
Since #42116 a Response or Request keeps a Blob-backed stream as its body, so text() reads that stream through the buffered fast path. Cover new Response(slice.stream()), new Response(response.body) and new Request(url, { body: response.body }), and the stream of new Blob([slice]).
1146abb to
412cca9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The guard reads as one named condition under a one-line comment. The window table reads a prefix, a prefix of a prefix, a suffix, a slice of a slice and two empty slices after their Blobs were collected, so each clause of the guard has a case that depends on it.
|
Updated 4:13 PM PT - Oct 2nd, 2026
@dylan-conway, your commit 68bbc7c is building: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The prefix-window cases added in 255bd1a (test/js/web/fetch/blob.test.ts windows matrix) now make the size >= len clause of the guard load-bearing, which closes my earlier comment. Beyond the inline findings, I also checked the let-else rewrite of to_internal_blob_if_possible against the old nesting — same three guards plus the new window check, single effect on the same path — and that blob.size.get() >= bytes.len() behaves correctly for the unset-size sentinel and an empty store, so an unsliced Blob still takes the fast path.
Extended reasoning...
The change narrows the zero-copy fast path in src/runtime/webcore/Blob.rs so a windowed Blob slice stays an Any::Blob and is read through the offset/size-respecting paths; it touches no security-sensitive surface beyond returning fewer bytes than before. The new test matrix in test/js/web/fetch/blob.test.ts covers sources x consumers, body text() reads, and edge windows including the prefix case I asked for last run. Inline findings from this run (a pre-existing same-class is_ascii flag issue and a comment nit) already signal that a human should look, so this body only records what was additionally ruled out.
| // A slice shares its parent's backing store. Once the parent and the slice | ||
| // objects have been collected, a stream made from the slice holds the only | ||
| // reference to that store, and the buffered consumers (Bun.readableStreamTo*, | ||
| // stream.text(), text() of a Response or Request whose body is that stream) | ||
| // take a fast path that hands the store itself to JS. That path used to ignore | ||
| // the slice's offset and size and return every byte in the store. |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers reading this suite get a six-line comment that narrates the old bug ("That path used to ignore the slice's offset and size and return every byte in the store") rather than what the code cannot say. The repository guidance asks for one-line comments that never narrate the change and prefers a GitHub issue link. Fix: cut the block at test/js/web/fetch/blob.test.ts:613-618 to one line stating the invariant (a stream made from a slice must yield only the slice's window once the Blob objects are collected) plus the issue link, and shorten the sibling block at :693-694 the same way.
Why this was flagged
The new describe block at test/js/web/fetch/blob.test.ts:619 is preceded by a comment at :613-618 that describes the pre-fix behaviour of Any::to_internal_blob_if_possible and the fast path it took, i.e. it narrates the change rather than a fact the test code cannot express. The repository review instructions state comments must be one line, never narrate the change, and prefer links to GitHub issues. No runtime behaviour is affected; on the base branch neither the comment nor the tests exist. A fix keeps the tests and reduces the comment to the invariant under test and an issue reference.
Verification: nit. The comment exists at test/js/web/fetch/blob.test.ts:613-618. Its final sentence, "That path used to ignore the slice's offset and size and return every byte in the store.", narrates the pre-fix behaviour of Any::to_internal_blob_if_possible, and the block is six lines with no issue link. No runtime behaviour is affected; neither the comment nor the tests exist on the base.
There was a problem hiding this comment.
Agreed, the block will become one line that states the invariant. I hold that edit so that the head that was checked does not move for a comment. It goes in with the next push to this file. There is no issue to link, the report came from a fuzz run.
2 participants
Problem
Blobobjects are collected.await new Response(parent.slice(7, 13).stream()).text()gives"SECRET-public-SECRET", not"public".Any::to_internal_blob_if_possible(src/runtime/webcore/Blob.rs:6034) moves the whole buffer out of a store with one reference. It ignores the Blob'soffsetandsize.text()reads it this way.Bun.readableStreamToText(slice.stream())was already wrong in 1.4.0.Fix
to_internal_blob_if_possiblealso requiresoffset == 0 && size >= store length. A windowed Blob stays anAny::Blob.Any::Blobarms ofto_action_valueapply the window. The same calls take that path while the parent is alive.test/js/web/fetch/blob.test.ts(consuming a slice's stream). 31 of 37 cases fail on the unfixed 1.4.3 canary, all pass here. Notes list the other suites.Background
Store.slice()shares it and narrowsoffsetandsize. EachBlobobject holds one reference until it is finalized.blob::Anyis what a buffered stream consumer reads.Any::Blobviews a shared store.Any::InternalBlobowns aVec<u8>that JS adopts without a copy.to_blob_if_possible()call intext(), as the other body getters have. It repairs the three body cases only.Downsides
Any::to_promise.Notes
What reads wrong on main (4b02e10), after the parent and the slice are collected
Bun.readableStreamToText / ToBytes / ToArrayBuffer / ToJSON(slice.stream())slice.stream().text() / .json() / .bytes()res = new Response(slice); res.body; await res.text()await new Response(slice.stream()).text()await new Response(new Response(slice).body).text()await new Request(url, { method: "POST", body: res.body }).text()json(),bytes(),arrayBuffer(),blob()of those bodiesBun.readableStreamToBlob(slice.stream())The main column is from runs of a debug build of main and of the 1.4.3 canary (367d939). The release column is from the report and from the first revision of this PR (17 of 21 cases failed on 1.4.0).
Why only
text()of a bodyBodyMixin::get_text(src/runtime/webcore/Body.rs:1755) passes aLockedbody's stream toreadableStreamToText. The other getters callto_blob_if_possible()first, which lifts the windowed Blob back out of the stream and reads it through the Blob paths. Before #42116,Body::Value::from_jslifted the Blob out of a Blob-backed stream when the body was made, sotext()never saw the stream.The path
ByteBlobLoader::to_any_blob(src/runtime/webcore/ByteBlobLoader.rs:146) checks the window before its own whole-buffer shortcut (Store::to_any_blob). For a slice it returns a windowedAny::Blobon purpose.ByteBlobLoader::to_buffered_valuepasses it toAny::to_promise, thento_action_value, which ran the unguarded conversion for each action exceptblob(). The store's other references are the parent and the slice objects. After the GC finalizes them,has_one_ref()is true and the window was lost.>=and not==:shared_viewclamps asizethat is larger than the store, so such a Blob views all of it too.Tests
describe("consuming a slice's stream after the Blobs were collected"), 37 cases: 3 sources (slice.stream(),new Response(slice).body,new Blob([slice]).stream()) x 8 consumers, 4 bodies read bytext(), 7 windows, and two unsliced controls. The windows are a prefix, a prefix of a prefix, a suffix, a slice of a slice, an empty slice at the start, an empty slice in the middle, andslice(0). A prefix has offset 0 and a suffix ends where the store ends, so each clause of the guard has a case that depends on it. The test waits on aFinalizationRegistryfor each Blob, because the finalizer is what releases the store reference.Unfixed builds: 31 of 37 fail on the 1.4.3 canary (367d939,
USE_SYSTEM_BUN=1), and 27 of the first 32 failed on a debug build of main 4b02e10. The 6 that pass are the 3readableStreamToBlobcases (that action skips the conversion),slice(0), and the 2 unsliced controls.Suites run on the debug build of this branch
blob.test.ts(147 pass),body.test.ts(774 pass, 4 skip),body-clone.test.ts(85),body-mixin-errors.test.ts(13),streams.test.js(624),readable-stream-blob-consumed.test.ts,blob-cow.test.ts,blob-array-fast-path.test.ts,readablestreamtoarraybuffer.test.ts,sync-pull-fast-path.test.ts. All pass. The suites other thanblob.test.tsran before the guard got its name in the last commit, which does not change what the guard does.